enhance gc - #320
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #320 +/- ##
==========================================
- Coverage 63.15% 63.13% -0.03%
==========================================
Files 32 35 +3
Lines 1900 4001 +2101
Branches 204 486 +282
==========================================
+ Hits 1200 2526 +1326
- Misses 600 1210 +610
- Partials 100 265 +165 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| new_shard_id); | ||
|
|
||
| if (err == ShardError::NO_SPACE_LEFT) { | ||
| gc_manager()->submit_gc_task(task_priority::normal, |
There was a problem hiding this comment.
If this shard doesnt meet gc standard, why we want to GC it?
There was a problem hiding this comment.
there is a case that every chunk has no available space and has garbage, but all of them do not reach the gc threshold. as a result , no chunk can be selected to create shard and no chunk can be gc, and all the create shard will fail. this is case was found in storage hammer gc test.
here , I try to trigger a gc without checking the threshold of the shard to handle this case , so that new create shard can succeed if new available space is freed. this is a best effort gc.
There was a problem hiding this comment.
If there is nothing we can GC, this will be an expensive operation which copies over a chunk for no benefit... But its a minor issue
There was a problem hiding this comment.
I think we can keep it like this for now, and adjust it if necessary in the future
ed9e4a9 to
f8947e2
Compare
xiaoxichen
left a comment
xiaoxichen
left a comment
There was a problem hiding this comment.
Please add summary to commit message.
Also,
- Suggest moving GC to a dedicate mod (logdebugmod)
- Suggest similar pattern like RD_LOG so that task_id can be print more consistent across the code (avoid extra space/capital by mistake).
define GC_LOG(level, task_id, msg, ...) \
LOG##level##MOD(GC, "[task_id={}] " msg, traceID, identify_str(), ##__VA_ARGS__)
| } | ||
|
|
||
| if (!process_after_gc_metablk_persisted(gc_task_sb, valid_blob_indexes)) { | ||
| if (!process_after_gc_metablk_persisted(gc_task_sb, valid_blob_indexes, 0)) { |
There was a problem hiding this comment.
nit： do not use magic number 0
There was a problem hiding this comment.
inline static atomic_uint64_t _gc_task_id{1}; // 0 is used for crash recoveryf
for the simplicity , 0 is dedicated for crash recovery
1 add task_id for each gc task, which will be helpful to track a specific gc task 2 we need to release the chunk when fail to create shard, so that it can be gc
3 participants
1 add task_id for each gc task
2 fix a bug: when fails to create a shard, we need to release the chunk, so that it can be gc